feat(server-utils): Allow integrations to be part of marker#22094
Conversation
332ece3 to
31572d5
Compare
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
31572d5 to
0ab49df
Compare
size-limit report 📦
|
95c6874 to
c0dfebc
Compare
b341ad9 to
06971c5
Compare
e7262f3 to
6713fc3
Compare
| const require = createRequire(import.meta.url); | ||
| const packageRoot = dirname(require.resolve('@sentry/server-utils/package.json')); | ||
| // This avoids any backslash-escaping concerns on Windows | ||
| const resolvedRegisterModule = resolve(packageRoot, 'build/esm/orchestrion/index.js').replace(/\\/g, '/'); |
There was a problem hiding this comment.
Not 100% clear what all this nastiness is for!
There was a problem hiding this comment.
I had it before, like I had it now pushed. But clankers were alwys like "You can't do this! You need to do it like this because XY". Since I have no idea what is hacky in bundlers and what not I went for this. I moved now point directly to the package, which also works for now.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
6713fc3 to
0d56181
Compare
4465fbd to
b59ebba
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
I'm not sure the best way forward to be honest, but I think there are some conflicts to comb through between this and the other orchestrion bundler stuff.
b59ebba to
157e383
Compare
157e383 to
6755627
Compare
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6755627. Configure here.
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
Looks good. Only real feedback:
Rather than injecting this, you could export helpers from @sentry/server-utils/orchestrion that handles all of the below? It would save multiple imports?
(globalThis.__SENTRY_ORCHESTRION__ ??= {}).integrations ??= new Map();
import { ${exportName} } from '@sentry/server-utils/orchestrion';
import { getClient as __sentryGetClient } from '@sentry/core';You could export register<MODULE_NAME>Integration for each and that handles the registration?
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
Added a few comments on the other threads. Totally find to land as-is (pending rebasing to pull in new orchestrion integrations on develop, of course.)
I agree with @timfish's suggestion that the snippet could be trimmed up a bit, but I think we could make it even a bit more tight/bulletproof/deterministic by defining a helper method in server-utils to handle it.
I took a stab such a thing in isaacs/jp/orchestrion-integrations if you want to take a look, or you can git am the commit from here: https://gist.github.com/isaacs/190b6d5c2fbeb2da6a1a96c2bb5f8df4
I don't think that should be blocking if you don't think it's a clear win, though. This is good to land imo :) 👍
…ethod Add a helper method to streamline the string snipped used by orchestrion bundler integrations. This removes the `getClient` import from `@sentry/core`, without adding a hand-maintained export for each integration.
713422f to
022aa6a
Compare
3 participants
This adds a way to add orchestrion integrations into the bundle during the build time and dev for only that things which are instrumented by Orchestrion.
Problem
We can't use
Sentry.experimentalUseDiagnosticsChannelInjection(), because the bundle would blow up, even if only one integration would be needed.I was in between adding a new
transformto every single config, but that would have meant that the bundle size would increase for the runtime config, which I didn't want to do, so I added atoSubscribeInjectionsfunction that is separately exported from all configs (not sure if that scales well)Solution (for now)
__SENTRY_ORCHESTRION__.integrationsis now added with a list of integrations via a virtual import with side effects. TheregisterChannelIntegrations()is actually here to do that. (In the future this list can be adapted in the best case, depending on what is really needed).This is explicitly not adding anything into Cloudflare to keep it small, this is in the PR stack further up.